This attribute set has no mcp_server, and neither does the one in recordOperationDuration just below. The three deleted proxy metrics (toolhive_mcp_requests, _request_duration, _tool_calls) all carried server, so per-server breakdown on request metrics goes away entirely. Only stacklok.toolhive.proxy.active_connections still has it. This needs sorting before merge.

The migration guide says otherwise at docs/telemetry-migration-guide.md:281, which maps server (proxy + rate limit metrics) to mcp_server with no caveat. That's accurate for the rate-limit metrics and wrong for these two.

Two ways out: add coremetrics.LabelMCPServer here and in recordOperationDuration, or fix that guide row and say per-server breakdown now relies on scrape-target labels or a target_info join. I'd lean toward the label, since one Prometheus scraping several proxies otherwise can't split them without a join.

Worth noting the e2e assertion for mcp_server="<name>" in test/e2e/osv_authz_test.go still passes, because active_connections supplies it. So the tests don't catch this.

Fixed in 46864d8 — added coremetrics.LabelMCPServer to both recordHTTPServerDuration and recordOperationDuration, and fixed the migration guide row that claimed server→mcp_server applied unconditionally. Also strengthened the assertions in TestHTTPMiddleware_HTTPServerRequestDuration to check for the label going forward.

This records the whole SSE connection lifetime into http.server.request.duration, which is on BucketsFastHTTP() and tops out at 10s. SSE connections live for minutes or hours, so every one lands in +Inf and any quantile over the unfiltered metric stops meaning much.

The example dashboards only use _count, so they're fine today. But docs/observability.md:234 documents the SSE behavior without mentioning the effect, so someone will build a p95 panel on this and get a flat line pinned to the top bucket.

Either record the SSE observation into a long-running bucket set, or note in the docs that latency queries need to filter http_request_method="POST".

Fixed in 46864d8 — split SSE connection duration into a dedicated stacklok.toolhive.proxy.sse_connection.duration histogram on BucketsMCPProxy() (tops out at 300s), rather than sharing http.server.request.duration's 10s-capped buckets. Updated docs/observability.md with the new metric's section and a note on why they're kept separate. Rewrote TestHTTPMiddleware_HTTPServerRequestDuration_SSEBranch (now TestHTTPMiddleware_SSEConnectionDuration_SSEBranch) to assert http.server.request.duration is no longer emitted on the SSE branch and the new histogram is.

These are OTel attribute keys (stacklok.component, with a dot), but OwnershipLabels is documented as Prometheus constant labels and goes straight into promclient.WrapRegistererWith at prometheus.go:48.

It works today, though only because model.NameEscapingScheme defaults to UnderscoreEscaping and rewrites the dot at exposition time. A scraper negotiating escaping=allow-utf-8, which Prometheus 3.x can do, gets stacklok.component verbatim on the go_* and process_* series while OTel-native series keep stacklok_component. A dashboard filtering on stacklok_component then silently drops all runtime metrics.

TestUnifiedMeterStrategy_PrometheusOwnershipLabels says it guards against "a wrong-form key (e.g. an OTel dotted attribute key instead of a Prometheus label name)" and passes anyway, for the same escaping reason. Passing stacklok_component and stacklok_product directly here makes it robust, and makes that test mean what it says.

Confirmed and fixed in 46864d8. You're right that this was only accidentally safe under the default negotiation. I reproduced the split with escaping=allow-utf-8: runtime/process series exposed "stacklok.component" verbatim while OTel-native series stayed stacklok_component, since the OTel exporter's own attribute-to-label conversion is unconditionally underscore regardless of scrape negotiation. Switched OwnershipLabels to hardcoded Prometheus-form keys directly, with a comment explaining why the OTel dotted constants don't belong here. Also strengthened TestUnifiedMeterStrategy_PrometheusOwnershipLabels to negotiate escaping=allow-utf-8 in a second subtest and assert the dotted form never appears. Confirmed it fails against the pre-fix code and passes now.

The left column holds real Prometheus names and the right column holds OTel dotted names, so anyone pasting a value from the right column into PromQL gets zero results. This needs fixing whichever cutover path we settle on, otherwise the guide can't actually be followed.

Rows needing _total: 286, 287, 289, 290, 294, 298, 300. Rows needing _seconds plus _bucket/_sum/_count: 288, 291, 292, 295, 299, 315, 316. And stacklok.vmcp.optimizer.token_savings carries unit %, so it exposes as stacklok_vmcp_optimizer_token_savings_percent, which happens to match the old Prometheus name exactly and is worth saying out loud.

Lines 324 to 330 do the translation correctly for the two HTTP/MCP metrics, which makes the omission in the tables read as deliberate. A Prometheus-name column would fix it, or one sentence above the tables covering dots to underscores, _total, and unit suffixes.

Fixed in 46864d8. Added a Prometheus Name column to the mapping table with the correctly suffixed underscore form for every row (_total for counters, _seconds_bucket/_sum/_count for histograms, and the declared-unit suffix like _percent where applicable). Verified each suffix empirically by constructing the actual instrument kind and scraping it, rather than assuming the general rule holds for every case, since the unit suffixes (_percent, {tools} producing no unit infix) needed confirming.

Small thing: toolhive_vmcp_backend_requests_duration appears twice, here as a rename and at line 319 in the deleted table, whose preamble says there's no renamed successor. Its two siblings (toolhive_vmcp_backend_requests and _backend_errors) only appear in the deleted table, so this row looks like a leftover. Dropping it clears up the contradiction.

Fixed in 46864d8. Dropped the contradictory row from the mapping table; it's now listed only in the Deleted Legacy Metrics table where it belongs.

The parenthetical says health.Monitor "depends on the decorated client already existing", but buildHealthMonitor at core_vmcp.go:277 takes only cfg and deliberately passes the undecorated cfg.BackendClient. Its own comment at line 283 says as much: "Use the undecorated client so health checks do not emit backend-call telemetry."

So there's no construction-order cycle to work around. If buildHealthMonitor moves above the MonitorBackends call, the provider can be passed by value and HealthProviderSetter goes away entirely, along with its mutex, Set/get, the four-value return at line 102, and the if healthProviderSetter != nil block at core_vmcp.go:253.

Not blocking, but it's a fair bit of machinery for a problem that isn't there.

You're right that buildHealthMonitor uses the undecorated client, so there's no construction-order dependency forcing this ordering. Reordering it before MonitorBackends and passing the health.StatusProvider by value would remove HealthProviderSetter, its mutex, Set/get, and the four-value return entirely. Agreed this is a real simplification, but it touches construction ordering across core.New and is a separate change from this PR's rename scope. Leaving as-is for now and will follow up separately rather than bundling a construction-order refactor into this review pass.

Small thing: the comment lists five branches including "workflow-instrument creation failure", but the table has four cases and that one isn't among them. The newWorkflowInstruments error path at core_vmcp.go:202 is the one left unexercised. Either add the case or trim the comment.

Fixed in 46864d8. Trimmed the comment to only list the four exercised branches, and added a note explaining why the newWorkflowInstruments error path isn't covered: the OTel SDK doesn't return an error for a same-name instrument-kind conflict (confirmed empirically), so there's no input reachable through the public API that would trigger it.

HIGH — http.request.method is recorded raw: unbounded, attacker-controlled cardinality.

Go's net/http accepts any RFC 7230 token as a method, and /metrics plus the MCP port are unauthenticated by default here. curl -X ZZZZ1, -X ZZZZ2, … mints a permanently-retained series per distinct method — each multiplied across the bucket set — in both the SDK's in-memory aggregation and Prometheus.

OTel HTTP semconv mandates exactly the missing mitigation: methods outside the known set MUST be recorded as _OTHER, with the original kept on spans only (http.request.method_original, which should not go on the metric).

The deleted toolhive_mcp_requests carried the same raw method, so this isn't net-new — but it's reintroduced on a metric this PR presents as semconv-compliant, and it's now the one metric recorded on every request path.

Separately, semconv lists url.scheme as Required on http.server.request.duration, and network.protocol.version / server.address as recommended. httpProtocolVersion(r) and mapTransport() are already computed a few lines down for mcp.server.operation.duration, so the data is at hand. http.route should be added only from a bounded route pattern — never the raw r.URL.Path.

HIGH — this reverts #6144, and changes bucket edges on an already-shipped metric.

247480f16 ("Adopt core MCP semconv bucket preset in telemetry", #6144) landed on main and deliberately put this exact line on BucketsMCPSemconv(). This branch moves it to BucketsMCPProxy(). @aponcedeleonch flagged this as the live rebase conflict and it's still unresolved at a8c3103b.

toolhive-core's own doc comment says BucketsMCPSemconv exists "for emitters that promise semconv fidelity" and names mcp.server.operation.duration as the example — so a semconv-named metric should carry semconv buckets.

Beyond the revert, the edges themselves change: [0.01, 0.02, 0.05, 0.1, 0.2, 0.5, 1, 2, …] → [0.01, 0.05, 0.1, 0.25, 0.5, 1, 2.5, …]. Any histogram_quantile() over a range spanning the upgrade mixes two incompatible layouts and returns silently wrong values. The migration guide documents zero bucket changes.

pkg/ratelimit/observability.go:80 and pkg/vmcp/core/core_telemetry.go:57 make the same substitution without conflicting textually and want the same look. On the latter: toolhive-core documents BucketsLongRunning() as being "for long-running measurements such as sync, reconcile, or composite-tool durations" — naming that exact use case.

HIGH — this assertion cannot fail for the property it claims to check, and the documented metric name doesn't exist.

coremetrics.RegisterBuildInfo sets metric.WithUnit("1"), and the Prometheus translator appends _ratio to any gauge with unit "1". I reproduced this against this repo's pinned deps:

stacklok_build_info_ratio{commit="abc123",component="toolhive",version="v1.2.3"} 1

require.Contains(t, body, "stacklok_build_info") is satisfied by stacklok_build_info_ratio as a substring, so the test passes while docs/observability.md:271 documents a name that matches nothing. Any fleet dashboard joining on stacklok_build_info returns empty.

Anchor the assertion at line start (^stacklok_build_info(_ratio)?\{) so the exported name is pinned, and reconcile with the docs. The durable fix is upstream — drop WithUnit("1") for a unitless identity gauge — worth filing before this vocabulary reaches customer dashboards.

Also worth asserting the labels: build_info always observes 1, so component="toolhive" / non-empty version / non-empty commit are the entire point of the metric, and none of them is currently checked.

HIGH — this is now false, and it's the first thing an operator reads when deciding whether an upgrade is safe.

Six metrics are deleted, 15 renamed, 3 merged, and several label keys changed. The Backward Compatibility table two sections above explicitly states metrics are a hard cutover, no legacy fallback — this paragraph directly contradicts it.

The dual-emission reassurance is true only for span attributes. Suggest retitling to scope it explicitly, e.g. "Step 1: Upgrade — Span Attributes Need No Action", and stating plainly that metric-based dashboards and alerts do break, with a pointer to Step 2.

Related: Step 4 ("Update Dashboard Panels") still advises running old and new queries "side-by-side during migration to verify equivalence" — impossible for metrics with no dual emission. Worth rescoping to trace-based panels.

HIGH — this cardinality claim doesn't hold, and it will mislead operators into thinking the vector is closed.

recordOperationDuration (middleware.go:791) sets gen_ai.tool.name to resourceID — the same parsedMCP.ResourceID value, i.e. params.name straight from the client's JSON-RPC body. For tools/call and prompts/get the unbounded value simply moved to a different key on the same metric; only resources/read (the URI) genuinely lost it.

Since mcp.server.operation.duration is now the sole per-method metric after the twins were deleted, arbitrary params.name values grow the series set without bound.

Either keep gen_ai.* names on spans only, or bound them against the aggregated capability set the proxy already knows (emitting _OTHER for unresolved names). Either way the note needs correcting — as written it asserts a control that isn't there.

MEDIUM — this guard is a prefix of the string it guards, so the assertion is tautological.

metricActiveConnections == "stacklok_toolhive_proxy_active_connections", and the guard is strings.Contains(metricsBody, "stacklok_toolhive_proxy"). The first assertion can therefore never fail, and the second only runs when the gauge already happens to be present. If the middleware emitted nothing at all, the whole block is skipped and the test still passes.

The test already made a real request through the middleware, so both series must exist — dropping the if and asserting unconditionally is strictly stronger.

(The NotContains assertions above this are unconditional and genuinely valuable — this is only about the guarded positive half.)

MEDIUM — the regex was weakened and the helper still can't fail.

The pattern went from toolhive_mcp_requests_total\{.*server="X".*transport="Y".*\} (\d+) to mcp_server_operation_duration_seconds_count\{[^}]*\} (\d+), dropping both the server-name and transport pinning. The surrounding if len(matches) > 0 { … } guard is left in place, so if the regex matches nothing the entire body — including Expect(totalRequests).To(BeNumerically(">", 0)) — is skipped and the helper passes silently.

Expect(matches).ToNot(BeEmpty(), "expected ... series in scrape") makes it capable of failing. Now that 46864d8e has restored mcp_server on this metric, the server-identity pinning can also come back.

MEDIUM — no zero-match guard after narrowing the scan from many series to one.

This previously scanned every line containing toolhive_mcp; it now scans only stacklok_toolhive_proxy_active_connections. If that single gauge is missing from the scrape for any reason — instrument creation fell back to the new noop.Int64UpDownCounter{}, a rename, an exporter change — the loop body never executes and the helper passes without asserting anything.

Unlike analyzeTraceAttributes, whose caller asserts TracesGenerated > 0 downstream, there's no backstop here. A checkedLines counter plus Expect(checkedLines).To(BeNumerically(">", 0)) closes it.

MEDIUM — server-identity assertions were dropped without replacement, in the one test that scrapes a real vMCP /metrics.

The rewrite deleted assert.Contains(metrics, 'server="telemetry-vmcp"'), 'transport="streamable-http"', and 'mcp_resource_id="search-svc_search"', replacing them only with mcp_method_name= checks. So the central server → mcp_server rename this PR performs isn't verified end-to-end anywhere. Same in TestIntegration_TelemetryRunsBeforeClassificationRejection, where server="telemetry-ordering-vmcp" gave way to a status-code check.

mcp_server="telemetry-vmcp" now lands on stacklok_toolhive_proxy_active_connections and mcp_client_operation_duration_seconds, so asserting it is cheap. The transport= assertion is unaffected by this PR and could simply come back.

MEDIUM — the "What Is New" table contradicts the "What Changed" section above it.

"What Changed" was updated to describe three new histograms, but this table still lists only mcp.server.operation.duration and mcp.client.operation.duration. Missing:

• http.server.request.duration
• stacklok.toolhive.proxy.sse_connection.duration (added in 46864d8e)
• stacklok.build_info
• stacklok.vmcp.mcp_server.health — semantically new, not a rename of toolhive_vmcp_backends_discovered
• the new outcome label vocabulary

Worth adding here since this table is what a reader scans to find out what they gain in exchange for the queries they have to rewrite.